[test-improver] Improve tests for cmd tracing helpers by github-actions[bot] · Pull Request #7951 · github/gh-aw-mcpg

github-actions · 2026-06-23T00:19:40Z

Test Improvements: tracing_helpers_test.go

File Analyzed

Test File: internal/cmd/tracing_helpers_test.go
Package: internal/cmd
Lines of Code: 158 → 183 (+25 lines)

Improvements Made

1. New Test Coverage

✅ Added TestLogTracingWarnf — covers logTracingWarnf which had 0% coverage; verifies the function prefixes output with "Warning: " using the standard log package

2. Corrected Misleading Test

✅ Renamed "unreachable endpoint falls back to noop provider and emits warning" → "configured endpoint creates SDK provider without warning" to accurately reflect OTel HTTP OTLP's lazy-connection behavior
✅ Removed incorrect comment claiming otlptracehttp.New "performs a dial during provider creation" (HTTP OTLP is lazily connected; construction succeeds for any URL)
✅ Removed conditional if warnMsg != "" { assert.Contains(...) } guard — this silently allowed the assertion to be skipped on every run; replaced with unconditional assertions
✅ Added assert.True(t, provider.IsEnabled()) — verifies that a configured endpoint produces a real SDK provider (not a noop), which is the correct expectation

3. Expanded SDK Provider Coverage

✅ Added "sdk provider shuts down cleanly" subtest to TestShutdownTracingProviderWithTimeout — the existing test only covered the noop provider path; the new subtest creates a real SDK provider and verifies clean shutdown

Coverage Improvement

Function	Before	After
`logTracingWarnf`	0%	100%
`initTracingProviderWithFallback`	62.5%	62.5% (error path requires SDK internals to fail)
`shutdownTracingProviderWithTimeout`	71.4%	71.4% (error path requires Shutdown to return an error)

Note: The remaining uncovered branches in initTracingProviderWithFallback and shutdownTracingProviderWithTimeout are error paths that require the OTel SDK to return errors (e.g., exporter construction or shutdown failure). These cannot be triggered in unit tests without mocking the SDK — the HTTP OTLP exporter is lazy and always succeeds at construction time.

Test Execution

All tests pass:

=== RUN   TestInitTracingProviderWithFallback
=== RUN   TestInitTracingProviderWithFallback/nil_config_returns_noop_provider_without_error
=== RUN   TestInitTracingProviderWithFallback/valid_config_with_no_endpoint_returns_noop_provider_without_warning
=== RUN   TestInitTracingProviderWithFallback/configured_endpoint_creates_SDK_provider_without_warning
--- PASS: TestInitTracingProviderWithFallback (0.00s)
=== RUN   TestShutdownTracingProviderWithTimeout
=== RUN   TestShutdownTracingProviderWithTimeout/noop_provider_shuts_down_cleanly
=== RUN   TestShutdownTracingProviderWithTimeout/sdk_provider_shuts_down_cleanly
--- PASS: TestShutdownTracingProviderWithTimeout (0.00s)
=== RUN   TestLogTracingWarnf
--- PASS: TestLogTracingWarnf (0.00s)
ok  github.com/github/gh-aw-mcpg/internal/cmd  0.012s

Why These Changes?

tracing_helpers_test.go was selected because logTracingWarnf was at 0% coverage despite being a simple, directly-testable function. The existing "unreachable endpoint" test also contained a subtle bug — the conditional assertion if warnMsg != "" meant the assertion never ran (since HTTP OTLP never fails lazily), so the test passed vacuously. This is a real correctness issue: the test appeared to validate error-fallback behavior but was silently a no-op. The fix removes the conditional and accurately documents the actual behavior.

Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

index.crates.io

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "index.crates.io"

See Network Configuration for more information.

Generated by Test Improver · 1.6K AIC · ⊞ 29.5K · ◷

- Add TestLogTracingWarnf to cover the previously untested logTracingWarnf function (was 0% coverage, now 100%) - Replace misleading 'unreachable endpoint falls back to noop' test with an accurate test that documents HTTP OTLP's lazy-connection behavior: the exporter construction succeeds even for unreachable endpoints and the provider is a real SDK (non-noop) instance - Add 'sdk provider shuts down cleanly' subtest to TestShutdownTracingProviderWithTimeout to cover the SDK provider code path (previously only the noop path was tested) - Strengthen assertions: remove conditional 'if warnMsg != ""' guard that silently allowed assertions to be skipped; all assertions are now unconditional Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Improves unit test coverage and correctness for the internal/cmd tracing helpers, including adding direct coverage for logTracingWarnf and adjusting tracing-provider tests to reflect OTLP/HTTP exporter lazy-construction behavior.

Changes:

Added TestLogTracingWarnf to cover the Warning: prefix behavior emitted via the standard log package.
Updated the “unreachable endpoint” init-provider test to instead assert SDK provider creation without warnings (reflecting lazy OTLP/HTTP exporter construction).
Added an SDK-provider shutdown subtest to TestShutdownTracingProviderWithTimeout.

Show a summary per file

File	Description
internal/cmd/tracing_helpers_test.go	Expands and corrects tests for tracing helper initialization, shutdown, and warning logging.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 1/1 changed files
Comments generated: 2

-		}
+		require.NotNil(t, provider, "Provider must not be nil")
+		assert.False(t, warnCalled, "OTLP exporter construction is lazy; no warning expected")
+		assert.True(t, provider.IsEnabled(), "Configured endpoint should produce an SDK (non-noop) provider")


+		provider, err := tracing.InitProvider(context.Background(), &config.TracingConfig{
+			Endpoint: "http://127.0.0.1:14318",
+		})
+		require.NoError(t, err)
+		require.True(t, provider.IsEnabled(), "Expected a real SDK provider with a configured endpoint")
+
+		var warnCalled bool
+		shutdownTracingProviderWithTimeout(provider, func(format string, args ...any) {
+			warnCalled = true
+		})
+		assert.False(t, warnCalled, "SDK provider with no pending spans should shut down without error")


lpcox · 2026-06-23T19:57:20Z

@copilot address review feedback

Copilot · 2026-06-23T20:00:18Z

@copilot address review feedback

Added t.Cleanup(func() { otel.SetTracerProvider(noop.NewTracerProvider()) }) to both subtests that install a real SDK provider as the global OTel tracer provider — the "configured endpoint creates SDK provider without warning" subtest in TestInitTracingProviderWithFallback and the "sdk provider shuts down cleanly" subtest in TestShutdownTracingProviderWithTimeout. This ensures background batcher goroutines are cleaned up and the global is reset to noop regardless of test ordering. Addressed in commit Fix global OTel provider leak in SDK tracing subtests.

github-actions Bot added automation improvement testing labels Jun 23, 2026

This was referenced Jun 23, 2026

[smoke-safeoutputs] Smoke Safe-Outputs PRs: 27995413561 #7956

Closed

[smoke-safeoutputs] Smoke Safe-Outputs PRs: 28029147317 #7976

Closed

lpcox marked this pull request as ready for review June 23, 2026 18:23

Copilot AI review requested due to automatic review settings June 23, 2026 18:23

Copilot started reviewing on behalf of lpcox June 23, 2026 18:23 View session

Copilot AI reviewed Jun 23, 2026

View reviewed changes

Copilot started work on behalf of lpcox June 23, 2026 19:57 View session

Fix global OTel provider leak in SDK tracing subtests

598d9f5

Copilot AI requested a review from lpcox June 23, 2026 20:00

Copilot finished work on behalf of lpcox June 23, 2026 20:00

lpcox merged commit 9fb4589 into main Jun 23, 2026
26 checks passed

lpcox deleted the test-improver/tracing-helpers-3d38ba02d5263da6 branch June 23, 2026 21:48

This was referenced Jun 23, 2026

[aw] No-Op Runs #6240

Open

[release] Release Highlights: v0.3.29 #8009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[test-improver] Improve tests for cmd tracing helpers#7951

[test-improver] Improve tests for cmd tracing helpers#7951
lpcox merged 2 commits into
mainfrom
test-improver/tracing-helpers-3d38ba02d5263da6

github-actions Bot commented Jun 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

lpcox commented Jun 23, 2026

Uh oh!

Copilot AI commented Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

github-actions Bot commented Jun 23, 2026

Test Improvements: tracing_helpers_test.go

File Analyzed

Improvements Made

1. New Test Coverage

2. Corrected Misleading Test

3. Expanded SDK Provider Coverage

Coverage Improvement

Test Execution

Why These Changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Copilot's findings

Uh oh!

lpcox commented Jun 23, 2026

Uh oh!

Copilot AI commented Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants